Bound SV2 extended-channel extranonce negotiation - #1854
Conversation
Test Results 2 files 2 suites 1s ⏱️ Results for commit 215077d. |
| #define SV2_MSG_SET_TARGET 0x21 | ||
|
|
||
| #define SV2_MAX_MERKLE_BRANCHES 20 | ||
| #define SV2_MIN_EXTRANONCE_SIZE 2U |
There was a problem hiding this comment.
What's with the U suffixes?
| { | ||
| if (min_extranonce_size < SV2_MIN_EXTRANONCE_SIZE || | ||
| min_extranonce_size > SV2_MAX_EXTRANONCE_SIZE) { | ||
| return -1; |
There was a problem hiding this comment.
Returning -1 here will show SV2: Connection lost to the user, which is not very informative.
Edit: this is always called with min_extranonce_size of 2. So not sure why this is tested here? Why not just supply the constant into the function call, as on the other side it's a magic number.
| active_job->ntime, | ||
| asic_result->rolled_version, | ||
| extranonce_2, en2_len); | ||
| uint8_t en2_len = conn != NULL ? conn->extranonce_size : 0; |
There was a problem hiding this comment.
If conn == NULL, don't we just have to abort everything here? Now this check is rolled into the en2_len assignment.
| if (!conn) return; | ||
|
|
||
| uint8_t extranonce_2_len = conn->extranonce_size; | ||
| if (extranonce_2_len < SV2_MIN_EXTRANONCE_SIZE || |
There was a problem hiding this comment.
Should this be tested here, or where conn->extranonce_size is set? IMO it's too late here.
3 participants
Summary
Root cause and impact
The pool supplies
extranonce_sizeas 16 bits. It was accepted without a bound, narrowed to eight bits, and later used with fixed 32-byte stack arrays. Values above 32 could cause out-of-bounds access, while values such as 256 changed meaning during narrowing.Valid 2-32 byte extended channels are unchanged. No authentication or network-policy change is added.
Related work
Validation
git diff --checkAn encrypted SV2 pool smoke test remains desirable.